-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: Move GeosBlockExtractor from geos-posp to geos-proccessing #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Move GeosBlockExtractor from geos-posp to geos-proccessing #142
Conversation
Update to the last version of the main
Update to the last version of the main
Update to the last version of the main
Update to the last version of the main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an Enum and the outer-most property
Checks from the setters could be a plus. ➕ if not too heavy
| """Extract ElementRegions block from a GEOS output multiBlockDataset mesh.""" | ||
| super().__init__() | ||
|
|
||
| self.geosElementRegionsName: dict[ int, str ] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I think is an enum and eventhough should be class var and not instance var :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is indeed an enum and it is already defined in GeosOutputsConstants. I don't need to rewrite it.
| elif elementRegionId == 2: | ||
| self.well = multiBlockDataSet | ||
|
|
||
| extractedElementRegions: ExtractedElementRegionsMesh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be accessible outside of the class ? If not then decorate it with @property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be accessible outside of the class to get the result of the extraction
| "The logger already has an handler, to use yours set the argument 'speHandler' to True during the filter initialization." | ||
| ) | ||
|
|
||
| def applyFilter( self: Self ) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If returned value is not used then maybe drop it, else maybe raise an error with diagnostic
geos-processing/src/geos/processing/post_processing/GeosBlockExtractor.py
Show resolved
Hide resolved
Update to the last version of the main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great edition !!
| assert extractedVolume.GetNumberOfBlocks() == 2 | ||
|
|
||
| if extractFault: | ||
| assert extractedFault.GetNumberOfBlocks() == 2 | ||
|
|
||
| if extractWell: | ||
| assert extractedWell.GetNumberOfBlocks() == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we check for more than the number of block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I need to check if the block extracted are the good one with testing the name of the block. As I use vtkExtractBlock, I asume that the extracted block is correct with all the data ... that why I don't test theblock further.
Update to the laste version of the main
In the context of the code refactoring, this pr aims to:
Note: A future pr will clean all the test mesh